-
Notifications
You must be signed in to change notification settings - Fork 786
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix: Memory leak - Detached HTML elements #715
Conversation
I've added the fix for the auto-scroll detached element (#713 (comment)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to the unset
signal name, can you add a test to packages/interact/interact.spec.ts
to check that Interactable.unset()
and Interaction.destroy()
work correctly?
I actually really prefer `destroy()` now 😅. I might later rename `Interactable.unset()` and leave an alias, but for now I think the inconsistency is fine since the method is only used internally and there's no corresponding `Interaction.set()`.
|
@@ -30,7 +30,7 @@ test('Interaction constructor', (t) => { | |||
|
|||
for (const coordField in interaction.coords) { | |||
t.deepEqual(interaction.coords[coordField], zeroCoords, | |||
`nteraction.coords.${coordField} set to zero`) | |||
`interaction.coords.${coordField} set to zero`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks :D
Attempt to solve Memory leaks - Detached HTML elements
…and interactable reference from interactable target
A proposed solution for the issue #713
This solution adds a more complete cleaning routine to the
unset
method in order to remove all detached elements.Also contains a little compatibility fix for the
test.sh
script that was not working on my MacOs (Mojave 10.14.4).Cheers !